Usually, we get result type by function signature constraint, but spark decimal arithmetic is very complex, I'm not sure if we can represent the computation by string. If has, can anyone guide me?
Or can we send a function to signature, then compute the result type.
Now I compute the result type second times, it's not a good solution, and will cause function which revokes this function gets a wrong input argument type such as makeCheckOverflow.

And in Spark, the decimal arithmetic format is check_overflow(a * b), we will use the argument type in function apply, so it will be normal after this function.

Do you have some suggestions for here? @mbasmanova
You could just review the function signature, other part will be refactored after #4434 merge.

@jinchengchenghh

spark decimal arithmetic is very complex,

Would you share the details?

This is spark decimal result type computation https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L604

I don't know how to express the if in string constraint

This is the adjustPrecisionScale
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/types/DecimalType.scala#L171

@jinchengchenghh Is allowPrecisionLoss a session property? I imagine we would need to define 2 separate functions, one for allowPrecisionLoss=true and another for allowPrecisionLoss=false, and choose the right kind based on the value of allowPrecisionLoss.

  // When `spark.sql.decimalOperations.allowPrecisionLoss` is set to true, if the precision / scale
  // needed are out of the range of available values, the scale is reduced up to 6, in order to
  // prevent the truncation of the integer part of the decimals.
  protected def allowPrecisionLoss: Boolean = SQLConf.get.decimalOperationsAllowPrecisionLoss

Yes, it is default true, current implementation is based on default value.

  val DECIMAL_OPERATIONS_ALLOW_PREC_LOSS =
    buildConf("spark.sql.decimalOperations.allowPrecisionLoss")
      .internal()
      .doc("When true (default), establishing the result type of an arithmetic operation " +
        "happens according to Hive behavior and SQL ANSI 2011 specification, i.e. rounding the " +
        "decimal part of the result if an exact representation is not possible. Otherwise, NULL " +
        "is returned in those cases, as previously.")
      .version("2.3.1")
      .booleanConf
      .createWithDefault(true)

allowPrecisionLoss is a rarely used config, someone else interested in it can add the new function for allowPrecisionLoss=false, maybe in another PR.

Do you have suggestions to generate the result type by function adjustPrecisionScale? I don't have much context for string constrait which then is computed by expression::calculation::evaluate. @mbasmanova

@jinchengchenghh I'm not sure I understand this question. It might be better to create GitHub issue and move this discussion over there. Please, CC: @majetideepak and @karteekmurthys

throw VELOX_UNSUPPORTED instead of returning -1?

Use int128_t HugeInt::parse(const std::string& str)

Or you could also replace with int128_t build(uint64_t hi, uint64_t lo) from Hugeint.h.

Understand that string form is readable but slower.

In tests, we should favor readability as top performance is not important.

If possible these 2 can be merged.

No, T is int64_t while the variable type uses uint64_t, we cannot get template UT as uint64_t

Does it require templating? You are using specific types in the functions. You could simply overload the functions.
or
May be you can still merge convert<int64_t> and convert<int128_t>.

Do

typedef typename std::conditional<std::is_same_v<TInput, int64_t>,uint64_t,__uint128_t>::type UT;

You can switch the unsigned type depending on T.

Thanks for your guide, fixed.

What are these changes for? It would be nice to extract these into a separate PR and add a test.

This will enhance the function signature, so @majetideepak suggests me to add it in this PR, #4773 (comment)

Do you make sure we should extract these into a separate PR?

Hmm... Can you describe these change in the PR description? Are there tests for these? Can you give me a pointer?

https://github.com/facebookincubator/velox/pull/4613/files#diff-4baee9f82f9c347f753972415d345941d2c2a110b608d480b090650171da096bR379
It is used to compute the result scale in function getResultScale

std::string getResultScale(std::string precision, std::string scale) {
  return fmt::format(
      "({}) <= 38 ? ({}) : max(({}) - ({}) + 38, min(({}), 6))",
      precision,
      scale,
      scale,
      precision,
      scale);
}

Current Decimal multiply and divide test can cover <= and ? changes, I also extend the calculation to support basic operator like <.

If the computation is wrong, we cannot get the correct result type.

Got it. Let's extract these changes into a separate PR and add a test to velox/expression/tests/SignatureBinderTest.cpp to cover all of added functionality.

Are you adding new dependencies to Velox? Please, update PR description to clearly state that and explain why these are needed.

Here I include this header file, so we will need it. https://github.com/facebookincubator/velox/pull/4613/files#diff-4b21d26191cbbdb9bc90cafd83aed144a4483b1c594f224cf3bbe67f401f5f83R19
#include <boost/multiprecision/cpp_int.hpp>.
It will introduce int256

It would be nice to extract this change into a separate PR. This would make this PR smaller and easier to review.

Would you update the docs for existing "divide(x, y) -> double" to clarify what types of x and y it supports?

Can x and y be decimals with different precision / scale? It so, can you make this clear? What would be the precision / scale of the result? Please, clarify.

Please, document this class and its public methods.

Please, check coding style document and update the PR to comply. Here, the naming needs to be updated.